Skip to content

Deliver typed oversized completion refusals and advertise the wire cap - #1203

Merged
rmcdaniel merged 5 commits into
mainfrom
fix/server-1136-completion-response
Nov 13, 2026
Merged

rmcdaniel merged 5 commits into
mainfrom
fix/server-1136-completion-response

Conversation

@rmcdaniel

@rmcdaniel rmcdaniel commented Nov 13, 2026 •

Copy link
Copy Markdown
Member

Oversized workflow-task uploads can lose their typed413 when HTTP/1 closes the request reader while Python or Rust is still sending. Valid aggregate completions also lack an advertised route-specific budget, so SDK planners apply the ordinary limit and require unavailable external storage.

The server keeps a refused upload alive through a bounded discard (64MiB remaining bytes, two seconds), then sends the original typed refusal and closes the connection. Discovery advertises limits.max_workflow_task_completion_bytes from the same helper used by workflow-task and activity-continuation completion admission. Per-value limits and ordinary routes retain their existing caps. Shared vectors cover wire-size boundaries and optional-field negotiation; frozen PHP fixtures remain exact under a named native divergence. No upload lease or reference-placement authority is added for continuation commands.

This head includes accepted #1202 main for General’s single immutable3.9.1 correction cohort. The physical3.9 schema is unchanged. Runtime middleware/discovery bytes match reviewed919b0f08, and retained-command runtime bytes match accepted main88709ed4; the sole merge conflict preserved both named divergence entries.

Validation on combined838eca90:17 focused Rust controls pass (real fixed-length/chunked TCP413, bounded discard, exact-cap routes, valid300×10KiB fan-out, zero writes, discovery, legacy keys, validator markers and retry-reference ownership);410 combined Node checks pass. Clean source build used the pinned Rust toolchain/two cargo jobs. Public-boundary and regression-corpus policy checks pass; Avro encoding is unchanged. Security exact-head counter-review PASS is linked in comments.

Required full current-head CI still gates merge. Server then refreshes #1205 final notes/tree and completes native publication/scans/corpus/Compose/alias/final Release gates. PHP/Python/Rust installed within/over-cap worker acceptance remains required before closing #1136 or shared .github#228; source tests do not establish that outcome. Uploads exceeding either discard bound may still encounter a transport failure. No PHP Server release.

Refs #1136 and durable-workflow/.github#228.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Successor e69df7c corrects the authoritative fixture path selection found by Repository contracts and adds its explicit selection control. The affected aggregate/discovery/divergence suites pass116/116. Shared vectors now also record the PHP/Rust-agreed negotiation contract: absence alone keeps ordinary fallback; explicit invalid/null/zero/negative/non-integer values refuse completion planning before upload/submission; positive smaller caps are honored; ordinary routes ignore the unused field. Runtime bytes are unchanged from6a7d5972. Fresh full CI and Security review remain gates.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security review HOLD on exact e69df7c37d675681ca301b9eedc2398bc1a3ac18 for one contract mismatch.

resources/platform-protocol-specs/cluster-info-envelope.schema.json declares limits.max_workflow_task_completion_bytes with minimum: 0, but the new authoritative vectors and Server reference both define zero as explicitly invalid and require consumers to refuse completion planning before upload/submission. This makes an invalid zero value schema-valid while the portable consumer contract says it must fail closed.

Please make the schema require a positive integer (minimum: 1) and add/retain a schema control that rejects zero. This also aligns with the existing max_payload_bytes schema and the native producer's normal positive advertised value.

The earlier fixture-selection omission is fixed at this head: the exact vector path is present and the explicit selection control passes. My independent affected Node run passed 116/116. Runtime bytes remain unchanged from 6a7d5972; fresh required CI and published SDK delivery qualification remain separate gates.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security schema finding corrected at919b0f08: the optional advertised completion cap now has minimum1. A committed PHP schema control consumes the shared positive/invalid vectors through the existing OpenApiSchema validator; the actual Draft202012 JSON Schema also passes all10 present-field vectors locally. The116 affected Node checks pass. Runtime remains unchanged from6a7d5972. The PHP test still requires normal CI; no local PHP-suite result is claimed.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Authority clarification for SDK planning: the new wire cap covers ordinary workflow completion and activity continuation completion, but grants no external-payload upload lease authority. Native v1 completion context accepts only kind=activity, operation=complete, slot=[result]. Continuation command slots remain refused; workflow/v2 completion contexts remain unqualified. Rust has no continuation API and keeps its prior raw continuation offload path unchanged. PHP owns its existing continuation planner follow-up with result-only binding preserved.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security source/authority PASS for exact head 919b0f080ecf70c7d9b85da51fd32604fa9b2b2f.

The two review findings are resolved:

  • the new authoritative vector is explicitly selected by the Shared Server fixture workflow, with a regression that detects its omission;
  • max_workflow_task_completion_bytes is now an optional positive integer (minimum: 1). The committed validator control consumes the shared negotiation cases, so positive and positive-smaller values validate while null, zero, negative, float, string, bool, object and array values fail closed. I independently ran the affected Node suites (116/116) and Draft 2020-12 validation over all ten present-field cases.

I also reviewed the runtime/transport delta (byte-identical to 6a7d5972bd9faa0065ac23ec7b5164ea77c17811):

  • the aggregate cap is derived by the same helper used for discovery and applies only to the exact workflow-completion and activity-continuation completion routes;
  • per-value/ordinary limits and external-payload upload rules remain separate;
  • oversized pre-auth bodies are streamed/discarded without collection, bounded by 64 MiB remaining input and two seconds, return the typed 413, mark the HTTP/1 connection closed, and admit no durable state;
  • shared exact/over wire vectors, fixed/chunked loopback controls, stalled/byte-budget controls, parity divergence, and absence/invalid consumer semantics are present. No upload lease authority or capability is added.

This is source/authority clearance only. Exact-head required CI must finish green, and shared #228 remains open until published Server plus installed SDK Worker within/over-limit qualification succeeds. The draft must not be used to claim delivery or close the product failure. Any source/head change requires counter-review.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security clarified the continuation placement boundary: content-addressed upload authority and reference-placement authority are separate. An activity/result-bound reference must not substitute into a continuation command argument, even for identical bytes. Continuation command arguments remain inline with no new command upload or command-bound context; any external continuation still declines on native. PHP adds an equal-blob authority control; Rust keeps its unsupported continuation planner unchanged. This supersedes any earlier suggestion that ordinary configured uploads authorize those continuation slots.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Combined3.9.1 source is838eca909feeb63b4a56bf8a961735ac16e895d1 after merging accepted #1202 main88709ed4 into reviewed919b0f08. The only merge conflict was the native divergence registry: both explicit retained-intent and completion-cap entries are preserved. Runtime payload middleware/discovery files are byte-identical to919b0f08; retained-command Store/updates/intent files are byte-identical to accepted main. The410 combined aggregate/discovery/divergence/recorder/update-validation Node checks pass. A clean focused combined Rust build/control is running; fresh full normal CI and exact-head Security counter-review remain gates. General chose a single immutable3.9.1 source cohort; no tag/publication has started.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security PASS — exact combined source counter-review at 838eca909feeb63b4a56bf8a961735ac16e895d1 (tree 13ba93fe0bc8e703a91dcd84ab4cce1568abdba2).

I independently fetched the PR head and verified its two parents are reviewed #1203 919b0f080ecf70c7d9b85da51fd32604fa9b2b2f and merged/accepted #1202 88709ed4c54754cfc18ae8cc4938351b149885c1. git show --remerge-diff identifies exactly one manual conflict, scripts/conformance/server-parity/native-divergences.json. The resolution preserves both retained-command-intent-readback and completion-byte-limit-discovery, and updates the completion correction's since value to the intended combined native 3.9.1 cohort.

Exact preservation checks pass:

  • Native: deliver typed refusals for oversized workflow-task completions #1136 runtime payload_limits.rs is blob 62313c724c04471040654ee97561e600c87bc85a in both the reviewed parent and this head; the other payload discovery/runtime files are likewise byte-identical.
  • Native: durable signal and update command keys (.github#216) #934 store.rs is blob edb2c4e8a9afd279100cd5eb90e0ece305def109 in both merged main and this head; the command-intent/store/update runtime group is byte-identical.
  • the combined divergence registry parses and contains both entries; the cluster-info schema still requires a positive (minimum: 1) completion cap; the complete combined diff is whitespace-clean.
  • no 3.9.1 tag exists, so no immutable release identity has been reused.

This is a source/merge-resolution PASS, not permission to bypass the current required full CI or the owner's clean combined Rust controls. Merge only after those gates are green. Then refresh #1205 onto the accepted combined source and obtain exact-head review of its final 3.9.1 notes/tree before any tag, aliases, Compose publication, or GitHub Release. Shared #228 remains open until installed SDK/Worker boundary acceptance on the immutable published tuple.

@sonarqubecloud

Copy link
Copy Markdown

@rmcdaniel
rmcdaniel marked this pull request as ready for review November 13, 2026 11:15
@rmcdaniel

Copy link
Copy Markdown
Member Author

Clean combined Rust controls completed successfully on838eca90: six payload-limit/TCP/wire controls, one discovery control, eight command-key controls, validator retained-marker control and distinct external-reference retained-update control (17 total). The clean build used the pinned Rust toolchain, two cargo jobs,2CPUs/4GiB, source read-only/network none. Exact owned container is removed. Combined Node controls passed410/410. Security has independently passed838eca90; fresh full normal CI still gates merge and no published/installed acceptance is inferred.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Exact combined838eca909feeb63b4a56bf8a961735ac16e895d1 now has63 successful checks and one intended skip, no pending/failure. Security exact counter-review and clean17 Rust/410 Node controls are recorded above. Submitted to the required merge queue with this exact head. Server then refreshes #1205 against the accepted combined main for final notes/tree/history review before any3.9.1 tag or publication; #228 installed acceptance remains separate.

Merged via the queue into main with commit 51f92f4 Nov 13, 2026
64 checks passed
@rmcdaniel

Copy link
Copy Markdown
Member Author

Merged through the queue as51f92f4100c569710c7f12fab878f2f6b030b550 after the exact combined source passed Security and all required checks. Server is refreshing #1205 against that accepted main, preserving the two-file release diff and obtaining final exact-tree/history review before the3.9.1 tag. This source merge is not published-image or installed SDK acceptance; #1136/shared#228 remain open.

@rmcdaniel
rmcdaniel deleted the fix/server-1136-completion-response branch November 13, 2026 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant